Skip to content

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 6, 2025

Add PyUnstable_ThreadState_SetStack() and
PyUnstable_ThreadState_ResetStack() functions to set the stack base address and stack size of a Python thread state.


📚 Documentation preview 📚: https://cpython-previews--139668.org.readthedocs.build/

@encukou
Copy link
Member

encukou commented Oct 6, 2025

This generally matches what I came up with :)

The start argument might be confusing, as the place usage grows towards.
Why not use bottom & top as in the internals?

@vstinner
Copy link
Member Author

vstinner commented Oct 6, 2025

This generally matches what I came up with :)

Good!

The start argument might be confusing, as the place usage grows towards. Why not use bottom & top as in the internals?

APIs like make_fcontext() or sigaltstack() use void *stack_start_address and size_t stack_size.

pthread_attr_setstack() and pthread_attr_getstack() also use void *stack_start_address and size_t stack_size.

@vstinner
Copy link
Member Author

vstinner commented Oct 6, 2025

cc @markshannon @zooba

@encukou
Copy link
Member

encukou commented Oct 7, 2025

On systems like musl that just set top to the current stack pointer, PyUnstable_ThreadState_ResetStack has two issues:

  • calling it “often” will essentially prevent stack protection
  • returning from a function that called it will get you past the top of the stack, which should be fine but isn't really tested.

Would it be better to save the results of the initial _Py_InitializeRecursionLimits on the thread state, and have PyUnstable_ThreadState_ResetStack return to that?

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API looks good, but it can fail in a couple of ways.

@bedevere-app
Copy link

bedevere-app bot commented Oct 7, 2025

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Oct 7, 2025

Thanks for making the requested changes!

@markshannon: please review the changes made to this pull request.

@bedevere-app bedevere-app bot requested a review from markshannon October 7, 2025 10:27
@markshannon
Copy link
Member

markshannon commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

Yes. I approved your PR.

Add PyUnstable_ThreadState_SetStack() and
PyUnstable_ThreadState_ResetStack() functions to set the stack base
address and stack size of a Python thread state.
@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Can we please merge #139294 first before we add stuff on top of it?

PR gh-139294 has been merged. I rebased my PR on top of it.

I also modified my PR to use (base, top) internally instead of (start_address, size).

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Oh, TSan CI now fails with:

0:03:59 load avg: 5.34 [43/76/1] test_capi worker non-zero exit code (Exit code -6 (SIGABRT)) -- running (2): ...

python: Python/ceval.c:506: void tstate_set_stack(PyThreadState *, uintptr_t, uintptr_t):
Assertion `ts->c_stack_soft_limit < ts->c_stack_top' failed.

Fatal Python error: Aborted

@vstinner
Copy link
Member Author

vstinner commented Oct 7, 2025

Oh, TSan CI now fails

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants